PR Review: #4578 — Added new modifier to Focus plugin

Type: Feature
Verdict: Needs discussion

What's happening (plain English)

1. When you use x-trap on a dialog/modal, the underlying focus-trap library calls .focus() on the first focusable element when the trap activates.
2. The browser's native .focus() auto-scrolls to bring the focused element into view.
3. If you have a long, scrollable modal and the focused element is off-screen (say, below the fold), activating the trap scrolls the modal content to that element — which is disorienting.
4. This PR adds .preventscroll modifier to x-trap which passes { preventScroll: true } to focus-trap's options, which in turn passes it to the native .focus() call — preventing the auto-scroll.

The code change itself is 4 lines — just a passthrough of a focus-trap option. It works correctly.

The naming problem

Here's what concerns me. Alpine already has two scroll-related modifiers/methods in the focus plugin:

So .noscroll and .preventscroll on the same directive do completely different things, but their names suggest they're related. A user seeing x-trap.noscroll.preventscroll would be very confused about what each does.

The $focus magic uses .noscroll() for the same concept this PR calls .preventscroll. Ideally they'd share a name, but .noscroll is already taken on x-trap for a different purpose.

Other approaches considered

1. Rename to .nofocusscroll — More descriptive: "don't scroll when focusing." Avoids confusion with .noscroll. This is probably the best name if you want to merge this.
2. Make .noscroll do both — Have .noscroll prevent page scrolling AND focus scrolling. Users wanting to prevent scroll probably want both. Simpler API, but changes existing behavior (currently .noscroll doesn't prevent focus scrolling).
3. Don't merge — The use case is niche (Livewire re-renders in long modals). Zero community engagement (no reactions, no comments). Could be solved in userland by the Filament/Livewire layer.

Changes Made

• Added Cypress test for the .preventscroll modifier (verified it fails without the fix, passes with it)
• Fixed merge conflict with the inert feature from main (the PR was based on stale code — the old inert handling was duplicated with main's new onPostActivate approach)
• Improved docs: removed reference to "focus-trap" internals, fixed grammar

Test Results

24/24 focus tests passing (23 existing + 1 new). CI was passing on the original PR. Build succeeds.

Code Review

• packages/focus/src/index.js:119-121: The implementation is clean — 3 lines, checks modifier, sets option. No concerns.
• The PR had no tests. I added one that creates a scrollable container with a button below the fold and verifies scrollTop stays at 0 when the trap activates.
• Docs referenced "focus-trap" by name — fixed to abstract away the implementation detail.

Security

No security concerns identified.

Verdict

Needs discussion. The implementation is correct and minimal, but the naming needs a decision before merging. Having .noscroll and .preventscroll on the same directive doing different things is confusing and will generate issues/questions.

My recommendation: if you want this feature, rename it to .nofocusscroll to clearly distinguish it from .noscroll. Or consider making .noscroll handle both concerns (preventing page scroll AND focus scroll), since users who prevent scroll likely want both behaviors.

Zero community engagement (no reactions/comments in ~11 months) suggests this isn't a widely-needed feature, so "no for now" is also reasonable.

Reviewed by Claude